Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(ui-dom-utils): update findFocusable logic #1223

Conversation

NamNg1121
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jun 19, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


instructure-ui-ci seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@NamNg1121 NamNg1121 changed the title WIP(ui-dom-utils): update findFocusable logic refactor(ui-dom-utils): update findFocusable logic Jun 19, 2023
@github-actions
Copy link

Preview URL: https://1223--preview-instui.netlify.app

@matyasf matyasf requested a review from HerrTopi June 19, 2023 14:37
@NamNg1121 NamNg1121 self-assigned this Jun 19, 2023
@@ -83,12 +83,7 @@ function findFocusable(

function hidden(element: Element | Node) {
const cs = getComputedStyle(element)
return (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refers to a ticket on the Slack channel. After researching the issue, I discovered that the developers who were implementing the feature had left. Therefore, I decided to remove the feature and test how it affected the components. Overall, everything was fine in the manual test. I suggest writing unit tests.

@matyasf matyasf self-requested a review July 21, 2023 09:59
@matyasf matyasf assigned matyasf and unassigned NamNg1121 Jul 21, 2023
@matyasf matyasf force-pushed the INSTUI-3786-update-focusable-component-for-testing-environments branch from ce51a7a to 01e4ef8 Compare July 21, 2023 10:02
@matyasf matyasf removed their assignment Jul 21, 2023
…display is 'none'

The code of findFocusable was lifted from some ancient JQuery UI helper. For some unknown reason it
treats an element as non-focusable if its CSS display prop is not inline and its size is 0 or less.
It seems to work fine without this criteria, and we could not find any reason why this clause was
added in the first place.
@matyasf matyasf force-pushed the INSTUI-3786-update-focusable-component-for-testing-environments branch from 01e4ef8 to c41a3ee Compare July 21, 2023 10:24
@matyasf
Copy link
Collaborator

matyasf commented Jul 21, 2023

The code of findFocusable was lifted from some ancient JQuery UI helper class. For some unknown reason it treats an element as non-focusable if its display prop is none (this is OK) OR its CSS display prop is not inline and its size is 0 or less.

It seems to work fine without this second criteria, and we could not find any reason why this clause was added in the first place (even when looking at JQuery UI's commit log).

@HerrTopi HerrTopi merged commit 69b3949 into master Jul 21, 2023
4 checks passed
@HerrTopi HerrTopi deleted the INSTUI-3786-update-focusable-component-for-testing-environments branch July 21, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants